Repository navigation
Conversation
The Service Registry services and service components, the TRS tools list, the TRS-Filer tool form and the DRS objects list read `provider` only in firstUpdated. React sets it on a server-rendered element after that, so they never loaded data. They now switch provider whenever `provider` or `baseUrl` changes: a custom provider takes precedence over `baseUrl` (as documented), and the base URL is used once the provider is removed. An unchanged provider or base URL no longer triggers a second load right after the first update. Adds tests for these components and the test scripts their packages lacked (unique ports 8003-8006). Closes #565 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Drop the comments of the live previews that explained why they pass `baseUrl` instead of `provider`, as the components now react to both. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A load from a provider that was replaced before it finished no longer overwrites the data, error or loading state of the new provider. The TRS tools and DRS objects lists start on their first page with a new provider. Removing the provider without a base URL clears the data and shows the missing-provider error (the TRS-Filer form clears its tool classes) instead of keeping the removed provider. Tests cover the base URL path, slow loads from a replaced provider and the removal of the provider. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Removing the provider while a load runs left the loading state set, as the dropped load no longer clears it. Also show the missing service ID error when the service ID is cleared, and drop filtered lists of a replaced provider in the Service Registry list. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Reviewer's GuideUses of the APIRemove preview comments that described the former base-URL workaround now that these examples can use components that respond to provider changes.
ImplementationUpdate the TRS-Filer tool-create component to react to provider and base-URL changes, reuse its own REST provider, and ignore loads from replaced providers.
Update the DRS objects component to switch providers, restart pagination on a switch, clear state when no provider remains, and drop stale loads.
Update the Service Registry single-service component to react to provider changes, clear missing-provider or missing-service state, and discard results from replaced providers or service IDs.
Update the Service Registry services component to switch providers, clear its list when neither provider is available, and ignore stale loads and filtering results.
Update the TRS tools component to switch providers, restart at the first page, clear missing-provider state, and ignore stale list and tool-class loads.
ConfigurationAdd test commands with distinct ports for the four packages and limit the root test run to three concurrent suites to avoid overloading CI browsers.
DocumentationDocument that providers may be set or replaced after rendering and recommend keeping a provider instance stable across React renders.
TestsTest TRS-Filer tool-create provider changes, base-URL behavior, provider removal, and stale-load handling.
Test the DRS objects component's provider switching, pagination reset, removal behavior, and stale-load handling.
Test REST DRS provider requests, errors, overridden fetch behavior, paging, and expanded object requests.
Test Service Registry single-service provider changes, URL fallback, service-ID updates, removal, and stale-load handling.
Test the Service Registry services component's provider switching, removal, URL fallback, failed loads, and stale-result handling.
Test REST Service Registry provider URLs, errors, response parsing, and overridden fetch behavior.
Test TRS tools provider switching, pagination reset, base-URL behavior, removal, and stale-load handling.
Test REST TRS provider requests, errors, overridden fetch behavior, paging and filters, and descriptor-path fallback.
Test REST TRS-Filer provider requests, errors, overridden fetch behavior, descriptor-path fallback, and creation methods.
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Documentation build overview
|
TES now uses test port 8003: the new test scripts move to service registry 8004, trs 8005, trs-filer 8006 and drs 8007. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #577 +/- ##
==========================================
- Coverage 73.53% 72.75% -0.78%
==========================================
Files 33 51 +18
Lines 8123 14334 +6211
Branches 381 720 +339
==========================================
+ Hits 5973 10429 +4456
- Misses 2141 3890 +1749
- Partials 9 15 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
With nine packages testing in three browsers each, all at once, Firefox on the CI runners failed to open test pages in time (Node 22 job, twice in a row). Run at most three packages' tests at the same time. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
uniqueg
marked this pull request as ready for review
November 13, 2026 14:46
Contributor
Linked issuesChecked at commit 29510bd #565 fix: react to provider set after the first update · 5 of 5 met
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 9 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="packages/ecc-client-elixir-trs-filer/src/components/tool-create/tool-create.ts" line_range="378" />
<code_context>
- this.toolClasses = await this._provider.getToolClasses();
+ const toolClasses = await provider.getToolClasses();
+ // a load from a provider that was replaced in the meantime is dropped
+ if (provider !== this._provider) return;
+ this.toolClasses = toolClasses;
} catch (error) {
</code_context>
<issue_to_address>
**Stale provider failures reach consumers**
When a tool-class request rejects after its provider is replaced or removed, `loadToolClasses()` dispatches `ecc-tool-create-failed` from its catch without checking whether that provider is still active, so consumers receive an error for a provider no longer in use.
Guard the catch-path failure dispatch by checking that the request's provider is still current.
Also at `packages/ecc-client-elixir-trs-filer/src/components/tool-create/tool-create.ts:377`.
</issue_to_address>
### Comment 2
<location path="packages/ecc-client-elixir-trs-filer/src/components/tool-create/tool-create.ts" line_range="340-342" />
<code_context>
- if (changedProperties.has("baseUrl") && this.baseUrl) {
- this._provider = new RestTrsFilerProvider(this.baseUrl);
- this.loadToolClasses();
+ if (
+ (changedProperties.has("provider") || changedProperties.has("baseUrl")) &&
+ this.useProvider()
+ ) {
+ if (this._provider) {
</code_context>
<issue_to_address>
**Late providers get a false validation failure**
When the component first updates without a provider or base URL and receives a provider afterward, `firstUpdated()` dispatches `ecc-tool-create-validation-failed` before the later provider assignment can load successfully, so listeners receive a failure for a usable form.
Avoid dispatching the missing-provider validation failure during the initial update when a provider can still be assigned afterward.
Also at `packages/ecc-client-elixir-trs-filer/src/components/tool-create/tool-create.ts:345`.
</issue_to_address>
### Comment 3
<location path="packages/ecc-client-elixir-trs-filer/src/components/tool-create/tool-create.ts" line_range="378" />
<code_context>
- this.toolClasses = await this._provider.getToolClasses();
+ const toolClasses = await provider.getToolClasses();
+ // a load from a provider that was replaced in the meantime is dropped
+ if (provider !== this._provider) return;
+ this.toolClasses = toolClasses;
} catch (error) {
</code_context>
<issue_to_address>
**Older requests overwrite current results**
When a request is still pending while a provider is switched away and back to the same instance, or a newer request starts through the same provider, the provider-identity checks (and the service-ID check) do not distinguish request generations, so an older response passes and overwrites newer data after a provider switch, page or search change, or same-provider request. Callers see stale results.
Track the latest request generation for each load and discard responses from earlier generations.
Also at `packages/ecc-client-ga4gh-drs/src/components/objects-list/objects.ts:130-131`, `packages/ecc-client-ga4gh-service-registry/src/components/service/service.ts:132`, `packages/ecc-client-ga4gh-service-registry/src/components/services/services.ts:124`, `packages/ecc-client-ga4gh-service-registry/src/components/services/services.ts:141`, `packages/ecc-client-ga4gh-service-registry/src/components/services/services.ts:197`, `packages/ecc-client-ga4gh-trs/src/components/tools/tools.ts:151`, `packages/ecc-client-ga4gh-trs/src/components/tools/tools.ts:191`.
</issue_to_address>
### Comment 4
<location path="packages/ecc-client-elixir-trs-filer/src/components/tool-create/tool-create.ts" line_range="344" />
<code_context>
+ (changedProperties.has("provider") || changedProperties.has("baseUrl")) &&
+ this.useProvider()
+ ) {
+ if (this._provider) {
+ this.loadToolClasses();
+ } else {
</code_context>
<issue_to_address>
**Old provider data survives failed switch**
When a provider is replaced and its load fails before returning new data, `loadToolClasses` leaves the old classes and selected class ID in place; if the new provider’s class request fails, `handleSubmit` can validate against those old classes and send their tool class to the new provider. The list components likewise retain their prior results after a failed replacement load, so callers see rows from the old provider.
Clear provider-derived records and tool-class selection when switching providers, before starting the replacement load.
Also at `packages/ecc-client-ga4gh-service-registry/src/components/service/service.ts:95`, `packages/ecc-client-ga4gh-service-registry/src/components/services/services.ts:86`, `packages/ecc-client-ga4gh-trs/src/components/tools/tools.ts:114`, `packages/ecc-client-ga4gh-drs/src/components/objects-list/objects.ts:88`.
</issue_to_address>
### Comment 5
<location path="packages/ecc-client-ga4gh-service-registry/src/components/service/service.ts" line_range="108-110" />
<code_context>
+ let provider: ServiceRegistryProvider | null = this.provider ?? null;
+ if (!provider && this.baseUrl) {
+ provider =
+ this._provider instanceof RestServiceRegistryProvider &&
+ this._provider.baseUrl === this.baseUrl
+ ? this._provider
+ : new RestServiceRegistryProvider(this.baseUrl);
+ }
</code_context>
<issue_to_address>
**Removed custom provider stays active**
When a removed custom provider subclasses the REST provider and has the same base URL as the `baseUrl` property, `useProvider()` reuses the custom instance based on its class and base URL, so the component continues using the removed provider and its overridden request behavior instead of the ordinary REST provider.
Reuse the existing REST provider only when it was created for the `baseUrl` fallback, not when it came from the `provider` property.
Also at `packages/ecc-client-ga4gh-service-registry/src/components/services/services.ts:107-109`, `packages/ecc-client-ga4gh-trs/src/components/tools/tools.ts:135-137`, `packages/ecc-client-elixir-trs-filer/src/components/tool-create/tool-create.ts:361-363`, `packages/ecc-client-ga4gh-drs/src/components/objects-list/objects.ts:107-109`.
</issue_to_address>
### Comment 6
<location path="packages/ecc-client-ga4gh-service-registry/src/components/service/service.ts" line_range="81" />
<code_context>
+ (changedProperties.has("provider") || changedProperties.has("baseUrl")) &&
+ this.useProvider();
+ if (providerChanged && !this._provider) {
+ this.service = null;
+ this.loading = false;
+ this.error =
</code_context>
<issue_to_address>
**Cleared data is not announced**
When a consumer relies on the component’s change event to synchronize its own data and the provider is removed without a base URL, a parent listening for the component’s data-changed event receives no event when the provider is removed and the component clears its data. The parent retains and can continue using the previous provider’s service, services, tools, or objects.
Dispatch the corresponding data-changed event with the cleared value when clearing component state.
Also at `packages/ecc-client-ga4gh-service-registry/src/components/services/services.ts:89`, `packages/ecc-client-ga4gh-trs/src/components/tools/tools.ts:117`, `packages/ecc-client-ga4gh-drs/src/components/objects-list/objects.ts:91`.
</issue_to_address>
### Comment 7
<location path="packages/ecc-client-ga4gh-service-registry/src/components/services/services.ts" line_range="86" />
<code_context>
this.loadData();
}
</code_context>
<issue_to_address>
**Old filter choices survive provider failures**
When a provider switch is followed by a failure to load the new provider's service types or tool classes, the provider-switch branches start loading service types or tool classes without clearing the previous provider's metadata. If the new provider's metadata request fails, the catches only log the failure, leaving the old choices available for filtering against the new provider's data.
Clear provider-specific filter metadata when switching providers, and keep it cleared if the new metadata request fails.
Also at `packages/ecc-client-ga4gh-trs/src/components/tools/tools.ts:115`.
</issue_to_address>
### Comment 8
<location path="packages/ecc-client-ga4gh-service-registry/src/components/service/service.ts" line_range="95" />
<code_context>
this.loadData();
}
</code_context>
<issue_to_address>
**Old provider data survives failures**
When a component already has data and a replacement provider rejects its load, a replacement provider's load can fail before assigning new data, while the old service, list, or tool classes remain in component state. The caller sees data from the previous provider alongside the new load error, or stale choices in the form.
Clear provider-scoped data when switching providers, before starting the replacement load.
Also at `packages/ecc-client-ga4gh-service-registry/src/components/services/services.ts:87`, `packages/ecc-client-ga4gh-trs/src/components/tools/tools.ts:115`, `packages/ecc-client-ga4gh-drs/src/components/objects-list/objects.ts:88`, `packages/ecc-client-elixir-trs-filer/src/components/tool-create/tool-create.ts:345`.
</issue_to_address>
### Comment 9
<location path="packages/ecc-client-ga4gh-trs/src/components/tools/tests/tools.test.ts" line_range="148-150" />
<code_context>
+ ></ecc-client-ga4gh-drs-objects>`
+ );
+ await waitUntil(() =>
+ fetchStub
+ .getCalls()
+ .some((call) => String(call.args[0]).startsWith(`${BASE_URL}/objects`))
</code_context>
<issue_to_address>
**Tools list reload goes unchecked**
When a regression keeps `loadToolClasses()` running on a base-URL switch but skips `loadData()`, a base-URL switch that reloads tool classes but skips `loadData()` passes the `loads from the base URL and reloads when it changes` test: lines 147–150 wait only for `/toolClasses`, so the tools list is never checked.
Also assert that the `/tools` request is made for both the original and changed base URLs.
</issue_to_address>Sourcery assessment
Approval pending. 9 findings to address first.
Blocking findings: packages/ecc-client-elixir-trs-filer/src/components/tool-create/tool-create.ts:378, packages/ecc-client-elixir-trs-filer/src/components/tool-create/tool-create.ts:342, packages/ecc-client-elixir-trs-filer/src/components/tool-create/tool-create.ts:378, packages/ecc-client-elixir-trs-filer/src/components/tool-create/tool-create.ts:344, packages/ecc-client-ga4gh-service-registry/src/components/service/service.ts:110, and 4 more
Contributor
There was a problem hiding this comment.
Hey - I've found 12 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="packages/ecc-client-ga4gh-trs/src/components/tools/tools.ts" line_range="191" />
<code_context>
- this.toolClasses = await this._provider.getToolClasses();
+ const toolClasses = await provider.getToolClasses();
+ // a load from a provider that was replaced in the meantime is dropped
+ if (provider !== this._provider) return;
+ this.toolClasses = toolClasses;
} catch (error) {
</code_context>
<issue_to_address>
**Older requests overwrite current results**
When a provider or service ID changes away and back while an earlier request is pending, the provider-identity and service-ID checks accept the old request as current again, so its response or error can overwrite newer results or loading state; paginated lists can also show data for the wrong page. Callers see stale results or inconsistent loading state.
Track a request generation and ignore responses, errors, and loading updates from requests whose generation is no longer current.
Also at `packages/ecc-client-ga4gh-drs/src/components/objects-list/objects.ts:130-131`, `packages/ecc-client-ga4gh-service-registry/src/components/service/service.ts:124-125`, `packages/ecc-client-ga4gh-service-registry/src/components/service/service.ts:132`, `packages/ecc-client-ga4gh-service-registry/src/components/services/services.ts:140-141`, `packages/ecc-client-ga4gh-service-registry/src/components/services/services.ts:197`, `packages/ecc-client-elixir-trs-filer/src/components/tool-create/tool-create.ts:378`, `packages/ecc-client-ga4gh-trs/src/components/tools/tools.ts:152`, `packages/ecc-client-ga4gh-trs/src/components/tools/tools.ts:190`.
</issue_to_address>
### Comment 2
<location path="packages/ecc-client-ga4gh-service-registry/src/components/services/services.ts" line_range="86-87" />
<code_context>
this.loadData();
}
</code_context>
<issue_to_address>
**Old provider data remains visible**
When a replacement provider's data or service-type request fails, the load handlers set or log an error without clearing the previous provider's services or service types, so the component displays old-provider data alongside the error and callers can select stale records or classes.
Clear the services and service types when the provider changes, before starting the replacement loads.
Also at `packages/ecc-client-ga4gh-service-registry/src/components/service/service.ts:95`, `packages/ecc-client-ga4gh-trs/src/components/tools/tools.ts:114-115`, `packages/ecc-client-ga4gh-drs/src/components/objects-list/objects.ts:87-88`, `packages/ecc-client-elixir-trs-filer/src/components/tool-create/tool-create.ts:344-345`, `packages/ecc-client-ga4gh-service-registry/src/components/services/services.ts:85`.
</issue_to_address>
### Comment 3
<location path="packages/ecc-client-ga4gh-trs/src/components/tools/tools.ts" line_range="114-115" />
<code_context>
this.loadData();
}
</code_context>
<issue_to_address>
**Old tool classes remain selectable**
When a replacement provider's tool-class request is pending or fails, the component keeps the previous provider's classes in its filter or form, so callers can select classes that do not belong to the active provider.
Clear the tool classes when the provider changes, before loading the replacement provider's classes.
Also at `packages/ecc-client-elixir-trs-filer/src/components/tool-create/tool-create.ts:344-346`.
</issue_to_address>
### Comment 4
<location path="packages/ecc-client-elixir-trs-filer/src/components/tool-create/tool-create.ts" line_range="344-345" />
<code_context>
+ (changedProperties.has("provider") || changedProperties.has("baseUrl")) &&
+ this.useProvider()
+ ) {
+ if (this._provider) {
+ this.loadToolClasses();
+ } else {
+ this.toolClasses = [];
</code_context>
<issue_to_address>
**Old provider data remains visible**
When a provider is replaced and its load fails, the replacement load fails, the components keep the previously loaded data rather than clearing it when switching providers. The TRS-Filer tool form can continue offering the old provider’s tool classes, and the other components can display old provider data as if it belonged to the new provider.
Clear provider-owned data when switching providers, or explicitly mark retained data as belonging to the previous provider and avoid presenting it as the replacement provider’s result.
Also at `packages/ecc-client-ga4gh-service-registry/src/components/services/services.ts:85-87`, `packages/ecc-client-ga4gh-service-registry/src/components/service/service.ts:95`, `packages/ecc-client-ga4gh-trs/src/components/tools/tools.ts:112-115`, `packages/ecc-client-ga4gh-drs/src/components/objects-list/objects.ts:85-88`.
</issue_to_address>
### Comment 5
<location path="packages/ecc-client-elixir-trs-filer/src/components/tool-create/tool-create.ts" line_range="378" />
<code_context>
- this.toolClasses = await this._provider.getToolClasses();
+ const toolClasses = await provider.getToolClasses();
+ // a load from a provider that was replaced in the meantime is dropped
+ if (provider !== this._provider) return;
+ this.toolClasses = toolClasses;
} catch (error) {
</code_context>
<issue_to_address>
**Inactive provider failures are reported**
When a replaced provider's tool-class request rejects, `loadToolClasses` handles the rejection without checking whether that provider is still current, then logs and dispatches `ecc-tool-create-failed`. Callers see a failure from an inactive provider, even if the replacement provider loads successfully.
Check that the request's provider is still current in the catch path before logging or dispatching the failure.
Also at `packages/ecc-client-elixir-trs-filer/src/components/tool-create/tool-create.ts:379`.
</issue_to_address>
### Comment 6
<location path="apps/documentation/pages/docs/providers.mdx" line_range="46-52" />
<code_context>
// 4. Handle events/states as needed
```
+You can set or replace a component's provider at any time, also after the
+component has rendered (as with React, which sets it on a server-rendered
+element only after the first update): the component then loads its data from
+the new provider, and a load that the previous provider has not finished yet
+is dropped. Create the provider once and pass the same instance on every
+render (e.g. with `useMemo` in React), as each new instance makes the component
+load its data again.
+
## Common Use Cases
</code_context>
<issue_to_address>
**WES results contradict provider guidance**
When a provider-aware component without stale-request guards, such as WES runs, has a pending load when its provider is replaced, the old response is applied instead of dropped, so it overwrites the new provider's data despite the overview's guidance. Readers following the guidance still see stale results.
Guard WES runs' in-flight results against provider replacement, or narrow the overview's promise to components that discard stale requests.
</issue_to_address>
### Comment 7
<location path="packages/ecc-client-elixir-trs-filer/src/components/tool-create/tool-create.ts" line_range="344-345" />
<code_context>
+ (changedProperties.has("provider") || changedProperties.has("baseUrl")) &&
+ this.useProvider()
+ ) {
+ if (this._provider) {
+ this.loadToolClasses();
+ } else {
+ this.toolClasses = [];
</code_context>
<issue_to_address>
**Recovered forms retain a false failure**
When the component first updates without a provider or base URL, then receives a provider, `firstUpdated()` dispatches `ecc-tool-create-validation-failed` before the later update loads the provider's classes, so consumers receive a validation failure for a form that subsequently recovers.
Defer the missing-provider validation event until the component has completed its initial provider setup.
</issue_to_address>
### Comment 8
<location path="packages/ecc-client-ga4gh-trs/src/components/tools/tools.ts" line_range="117" />
<code_context>
+ this.loadData();
+ this.loadToolClasses();
+ } else {
+ this.tools = [];
+ this.loading = false;
+ this.toolClasses = [];
</code_context>
<issue_to_address>
**Consumers retain removed-provider data**
When a provider is removed after a consumer has received list data, the component clears its state without dispatching the corresponding changed event, so consumers that cache the last event payload keep displaying or acting on data from the removed provider.
Dispatch the corresponding changed event with the cleared state when the provider is removed.
Also at `packages/ecc-client-ga4gh-drs/src/components/objects-list/objects.ts:90-91`, `packages/ecc-client-ga4gh-service-registry/src/components/services/services.ts:88-89`.
</issue_to_address>
### Comment 9
<location path="packages/ecc-client-elixir-trs-filer/src/components/tool-create/tool-create.ts" line_range="347" />
<code_context>
+ if (this._provider) {
+ this.loadToolClasses();
+ } else {
+ this.toolClasses = [];
+ }
+ }
</code_context>
<issue_to_address>
**Provider removal goes unreported**
When the provider is removed while `baseUrl` is empty, `updated()` clears `toolClasses` but does not set a missing-provider error or dispatch `ecc-tool-create-validation-failed`, unlike the initial missing-provider path in `firstUpdated()`. Consumers are not notified when the provider disappears, and the form gives no indication that it can no longer submit.
Report the missing-provider state through the same validation event and user-visible error path used when the component first renders without a provider.
</issue_to_address>
### Comment 10
<location path="packages/ecc-client-ga4gh-service-registry/src/components/service/tests/service.test.ts" line_range="98" />
<code_context>
+ const firstCalls = first.getServiceById.callCount;
+
+ el.provider = second;
+ await waitUntil(() => second.getServiceById.callCount === 1);
+
+ expect(first.getServiceById.callCount).to.equal(firstCalls);
</code_context>
<issue_to_address>
**Replacement response goes unchecked**
When the replacement provider's result is not assigned to the displayed service, while its method is still called, the test `loads from a provider that replaces the first one` passes if the second provider is called but its result is never displayed: line 98 checks only the call count, and both providers return a service with ID `a` by default. It never asserts that `shown(el)` reflects the replacement provider's result.
Give the second provider a distinct service result and assert that `shown(el)` contains it after the replacement load.
</issue_to_address>
### Comment 11
<location path="packages/ecc-client-ga4gh-trs/src/components/tools/tests/tools.test.ts" line_range="157" />
<code_context>
+ await waitUntil(() =>
+ fetchStub
+ .getCalls()
+ .some((call) => call.args[0] === `${OTHER_URL}/toolClasses`)
+ );
+ });
</code_context>
<issue_to_address>
**Base-URL tool list can go unloaded**
When a base-URL change still loads tool classes but stops calling `getToolsList`, the test `loads from the base URL and reloads when it changes` passes if the component fetches tool classes but never loads or reloads the tools list: line 158 checks only for the new `/toolClasses` request. No assertion checks a `/tools` request or the resulting tools.
Also assert that the tools-list endpoint is requested for both base URLs, or verify that the resulting tools are loaded.
</issue_to_address>
### Comment 12
<location path="packages/ecc-client-elixir-trs-filer/src/components/tool-create/tool-create.ts" line_range="347" />
<code_context>
+ if (this._provider) {
+ this.loadToolClasses();
+ } else {
+ this.toolClasses = [];
+ }
+ }
</code_context>
<issue_to_address>
**Provider validation events are stale**
When the form first updates without a provider and React supplies one afterward, or a provider is later removed without a base URL, the first update emits `ecc-tool-create-validation-failed` before React supplies a provider, but the later update only loads tool classes; callers receive a false missing-provider event. Conversely, removing the provider later clears the classes without emitting a missing-provider event.
Synchronize the validation event with provider changes, emitting it only while neither a provider nor a base URL is available.
</issue_to_address>Sourcery assessment
Approval pending. 12 findings to address first.
Blocking findings: packages/ecc-client-ga4gh-trs/src/components/tools/tools.ts:191, packages/ecc-client-ga4gh-service-registry/src/components/services/services.ts:87, packages/ecc-client-ga4gh-trs/src/components/tools/tools.ts:115, packages/ecc-client-elixir-trs-filer/src/components/tool-create/tool-create.ts:345, packages/ecc-client-elixir-trs-filer/src/components/tool-create/tool-create.ts:378, and 7 more
From the automated review: when the new provider's load fails, the components no longer show the data, filter choices or tool classes of the previous provider; a failure of a replaced provider no longer fires the TRS-Filer form's failure event. Removing the provider fires the data-changed event with the cleared value. The components reuse only the REST provider they created for the base URL, not a custom provider that happens to be a REST provider for the same URL. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
From the automated review: the TRS-Filer form fires ecc-tool-create-validation-failed when its provider is removed without a base URL, as it does on the first update. The providers overview no longer says that loads of a replaced provider are dropped, as not all components do so. The service test checks the replacement provider's result. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This pull request adds test commands to the TRS, TRS-Filer, DRS and Service Registry packages, so CI now uploads their coverage, and their untested providers pulled the project coverage down by 4.5 % (Codecov allows 1 %). Test every method of the four REST providers: the requested URL, error responses, requests through an overridden fetch, paging and filters, the descriptor path fallback and the TRS-Filer creation methods. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Components that read
provideronly infirstUpdatedignored a provider set later, e.g. by React on a server-rendered element, and never loaded data. They now react to changes ofproviderandbaseUrl, as the Cloud Registry form and the WES components do:servicesandservice, TRStools, TRS-Filertool-create(the ones named in the issue), and DRSobjects(same pattern, found while checking all components with aprovider).baseUrl(as the JSDoc already said; before, a laterbaseUrlchange replaced it). Removing the provider falls back tobaseUrl; removing both clears the data and shows the missing-provider error.testscripts for the four packages that had none (unique ports 8004-8007, listed inweb-test.runner-config.mjs), so CI runs them.baseUrlworkaround are removed.Closes #565
Verification
npm run lint(includes the build of all packages and the docs): 19/19 tasks successful.npm run test: all packages pass in Chromium and Firefox (service-registry 14, trs 7, trs-filer 6, drs 6, cloud-registry 45, design 19, tes 51, wes 7), also after merging the latestdev. WebKit isn't installed on this machine, so the WebKit run is left to CI.devbefore the change).CI passed, Test on Node 22 and 24, Lint, CodeQL, Read the Docs, Sourcery,codecov/patch89.7%,codecov/project72.64%, -0.89% againstdev, within the 1% threshold ofcodecov.yml).codecov/projectfailed). Unit tests for their REST providers (TRS, TRS-Filer, DRS, Service Registry: every method's URL, error responses, an overriddenfetch, paging and filters, the descriptor path fallback, the TRS-Filer creation methods) bring it back within the threshold; the providers are now covered 99-100%. Run locally in Chromium and Firefox (WebKit is left to CI).Unrelated fixes
npm run testnow runs at most three packages' tests at once (--concurrency=3instead of--parallel): with nine packages testing in three browsers each, Firefox on the Node 22 runner failed to open test pages in time, twice in a row, in packages this PR doesn't touch (design, WES).Review
Regular lane: adversarial-reviewer and quality-reviewer, two rounds. Fixed: stale loads from a replaced provider, paging reset on a provider switch, behaviour when the provider is removed, a loading state left set after removal, more tests (base URL path, slow loads, removal).
Automated review (Sourcery)
Two rounds, all threads answered and resolved. Fixed: data, filter choices and tool classes of the previous provider are cleared when the new provider's load fails; failures of a replaced provider no longer fire the form's failure event; removing the provider fires the data-changed event (lists, service) or
ecc-tool-create-validation-failed(form); only the REST provider created forbaseUrlis reused; tests check the/toolsrequests and the replacement provider's result; the providers overview no longer promises that all components drop loads of a replaced provider. Declined: see below.Declined review findings
useProvider()helper instead of five copies: the client packages share no code, and a shared module would be a new dependency between packages (a tech decision for the maintainer). Each copy is about ten lines.satisfiesvs. a cast): the TRS, TRS-Filer and DRS providers have many methods that these components don't use, so those tests stub only what the component calls.runslets a laterbaseUrlreplace a custom provider, and TRStool, DRSobject, WESrunand DRS-Filerobject-createdon't fall back tobaseUrlwhen the provider is removed: outside this issue, left unchanged.filterServices) has no.catch, so a failed search leaves the loading state set; already ondev, and that code is being reworked in feat(service-registry): list filters and pagination #455. The provider guard added there has no test of its own for the same reason.firstUpdatedloads them after the list); harmless, unchanged.useProvider()copies must stay in sync; the identical shape makes that easy to check.ecc-tool-create-validation-failedon the first update when a provider may still come: unchanged existing behaviour, shared with the Cloud Registry form.servicestill loads twice on the first render (firstUpdatedplusupdatedforserviceId), as dotoolsandobjectsthroughpageSize: unchanged existing behaviour.Notes
services.tsor add atestscript to the service-registry package; whichever merges second needs a small rebase.ecc-client-elixir-drs-filerandecc-client-ga4gh-tesstill have notestscript.🤖 Generated with Claude Code
Summary by Sourcery
Make provider and base URL changes reactive across registry, TRS, TRS-Filer, and DRS components while preventing stale loads and improving test coverage and reliability.
Bug Fixes:
Enhancements:
CI:
Documentation:
Tests: